Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Update to latest Ruma version #4532

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Jan 14, 2025

Extracted from #4531.

This patch updates ruma to the latest commit at the time of writing.

I'm really not sure about what I'm doing, so please be careful during the review 😛.

@Hywan Hywan requested review from a team as code owners January 14, 2025 15:44
@Hywan Hywan requested review from bnjbvr and richvdh and removed request for a team January 14, 2025 15:44
@Hywan Hywan force-pushed the chore-update-ruma branch from cb8d562 to 7c6a592 Compare January 14, 2025 15:48
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.42%. Comparing base (9641aa9) to head (7c6a592).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4532      +/-   ##
==========================================
- Coverage   85.43%   85.42%   -0.02%     
==========================================
  Files         285      285              
  Lines       32105    32064      -41     
==========================================
- Hits        27429    27390      -39     
+ Misses       4676     4674       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@poljar
Copy link
Contributor

poljar commented Jan 14, 2025

This patch updates ruma to the latest commit at the time of writing.

Latest commit on the stable branch of Ruma or on the main branch? The main branch won't see a release in the next 6ish months.

The Ruma folks very nicely provide a stable branch on which they backport non-breaking changes: https://github.com/ruma/ruma/tree/ruma-0.12.

What exactly do you need from the Ruma bump?

@richvdh
Copy link
Member

richvdh commented Jan 14, 2025

I feel like this probably wants a changelog entry or two, assuming that the ruma changes result in some externally-visible change?

@Hywan
Copy link
Member Author

Hywan commented Jan 15, 2025

@poljar Last commit from main. We need this, ruma/ruma#1995.

@poljar
Copy link
Contributor

poljar commented Jan 15, 2025

@poljar Last commit from main. We need this, ruma/ruma#1995.

That has been backported to the release branch: ruma/ruma@b868438.

Please use that rev instead.

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo tweaking the Ruma repo source that @poljar asked for 👍

let mentions = original.content.mentions.clone();
let replied_to_original_room_msg =
extract_replied_to(source, room_id, original.content.relates_to).await;
let mentions = original.content.mentions;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we inline this variable now?

@@ -662,15 +583,12 @@ mod tests {

cache.events.insert(event_id.to_owned(), event);

let room_id = room_id!("!galette:saucisse.bzh");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't seem to like my galette saucisse 👿

Comment on lines 107 to 108
/// The content of the event to reply to.
content: ReplyContent,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the full content, I think this could store directly the thread information as an Option; then we could likely get rid of ReplyContent too, which would be nice!

@@ -57,7 +57,7 @@ proptest = { version = "1.5.0", default-features = false, features = ["std"] }
rand = "0.8.5"
reqwest = { version = "0.12.4", default-features = false }
rmp-serde = "1.3.0"
ruma = { git = "https://github.com/ruma/ruma", rev = "b266343136e8470a7d040efc207e16af0c20d374", features = [
ruma = { git = "https://github.com/ruma/ruma", rev = "5c9ef9f425d8f8b330e46d215c683bf799a28d50", features = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a comment should be added to only update Ruma using the ruma-0.12 branch, to make sure no one tries to update using the main branch before we plan to make a breaking release?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry you found it a chore.

Please can we have a changelog?

@poljar
Copy link
Contributor

poljar commented Jan 15, 2025

I'm sorry you found it a chore.

Can we tone down the snark please?

Please can we have a changelog?

All of the reviews are moot since this PR points to the main branch which has a bunch of breaking changes, those will be gone once we depend on the 0.12 branch of Ruma. So please a bit of patience, it's very likely that no significant changes happened due to this.

@richvdh
Copy link
Member

richvdh commented Jan 15, 2025

So please a bit of patience, it's very likely that no significant changes happened due to this.

Sorry, I didn't mean to rush things. I saw it had an approving review from Benji and thought it was ready to go other than the pending review from https://github.com/orgs/matrix-org/teams/rust-crypto-reviewers, so my intention was to turn my earlier comment (which I thought might have been missed) into a request for change.

If there are no significant changes, then of course it's fine to omit a changelog. (It feels a bit surprising that we'd bother updating ruma if it didn't result in some externally visible change, but I'm happy to take your word for it. I'd just like explicit confirmation that the lack of changelog is deliberate rather than accidental.)

@bnjbvr
Copy link
Member

bnjbvr commented Jan 15, 2025

For the record, we'd need some kind of changelog entry here to indicate that we stop supporting reply fallbacks in general; this is a breaking change in terms of behavior, for external users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants